Add a "Tag" model for event classification (Issue #161). #176
Conversation
In order to classify an event (for positioning on the home page or if users need to pay for it) a "Tag" model was created and linked to the "Event" model through a ManyToManyField. The tags are displayed as a CheckboxSelectMultiple widget to make it user-friendly.
ACM_General/events/models.py
Outdated
|
|
||
| #: The name of the tag that is displayed on the event creation form; represented | ||
| #: as a CharField. | ||
| display_name = models.CharField( |
There was a problem hiding this comment.
Should have unique=True, we don't want two tags have the same display name
ACM_General/events/models.py
Outdated
| ) | ||
|
|
||
| #: Unique id of the tag; represented as an IntegerField. | ||
| id = models.IntegerField( |
There was a problem hiding this comment.
What I was thinking for this id was another CharField which has an unchanging name that the creator of this Tag creates. This makes things look better from the API perspective when requesting event data.
ACM_General/events/models.py
Outdated
| Changes what is displayed for each tag on the event creation | ||
| form (without this it shows 'Tag object (id)'). | ||
|
|
||
| :return: String containing the name of the tag. |
There was a problem hiding this comment.
This should be :returns:. Also, The text should probably be "The display name of the tag". You don't need to mention the type because it is stated in the statement below.
There was a problem hiding this comment.
The other two functions just say :return: as well so I'll go ahead and change those too.
ACM_General/events/models.py
Outdated
| form (without this it shows 'Tag object (id)'). | ||
|
|
||
| :return: String containing the name of the tag. | ||
| :rtype: string. |
There was a problem hiding this comment.
No period, should be str.
ACM_General/events/models.py
Outdated
|
|
||
| #: If the tag should be diplayed on the event creation form; represented as a | ||
| #: BooleanField. | ||
| display = models.BooleanField( |
There was a problem hiding this comment.
I don't know if I necessarily agree with the name of this being display. The concept behind this was that if this field is true, the user can no longer add this tag to the event nor can it be seen on events that have already used this tag. This first part of this can be solved with https://stackoverflow.com/questions/3010489/how-do-i-filter-values-in-a-django-form-using-modelform#3013509 and a custom function on this model which is something like is_enabled(). I believe the name should be something like is_disabled. What do you think?
ACM_General/events/models.py
Outdated
| @@ -47,6 +47,46 @@ def get_path_for_flier(instance, filename): | |||
| ) | |||
|
|
|||
|
|
|||
| class Tag(models.Model): | |||
| """ | |||
| Class used for adding classification tags on events. | |||
There was a problem hiding this comment.
I would avoid using Class here and rephrase this docstring.
ACM_General/events/models.py
Outdated
|
|
||
| def __str__(self): | ||
| """ | ||
| Changes what is displayed for each tag on the event creation |
There was a problem hiding this comment.
I don't know that you necessarily need to put this description on this function. The __str__ attribute is a Python standard attribute which returns this object whenever str() is called on it. Moreover, these descriptions should avoid addressing django-specific projects and be specific to the function or return itself.
ACM_General/events/models.py
Outdated
| Class used for adding classification tags on events. | ||
| """ | ||
|
|
||
| #: The name of the tag that is displayed on the event creation form; represented |
There was a problem hiding this comment.
This will not exclusively be what it will be used for. It will be used anytime that we display the Tag. Rephrase to make more generic.
ACM_General/events/models.py
Outdated
| #: as a CharField. | ||
| display_name = models.CharField( | ||
| verbose_name=_('Event Tag'), | ||
| help_text=_('Different categories for the event.'), |
There was a problem hiding this comment.
This help text is not really descriptive as to what it does. Should be something along the lines of The name of the Tag that will be displayed to users
|
Two more generic comments is that the automatic tests of your code has failed and the content of your PR should say (below your paragraph): Closes: #161 |
dmgardiner25
force-pushed
the
event_tagging
branch
from
28eaaec to
a1f3ec4
Compare
Closes: #161